Fix undefined behavior of BitsSet.popFront() upon aggressive optimization#4509
Fix undefined behavior of BitsSet.popFront() upon aggressive optimization#4509wilzbach merged 1 commit intodlang:masterfrom
Conversation
|
Apparently this breaks on DMD's 32 bit targets. Seems valid at first glance, though, and it seems like there should be no way this causes a segfault?! |
|
Weird errors. :( |
|
Don't you need to increment |
|
Why? I thought when popFront leaves the range empty, front() can return garbage? |
|
I'm just looking at it from a perspective of having identical behavior as before. |
|
You still have a call to |
That would be |
|
Yeah, you are right. |
Nice!! thanks. |
|
Now that value cannot be 0 for the |
JohanEngelen
force-pushed
the
outofboundshift
branch
from
a166c9b to
d16cc66
Compare
|
@schveiguy updated! |
|
Well, LGTM. Let's hope it passes the 32-bit checkers. |
std/bitmanip.d
Outdated
| { | ||
| import core.bitop : bsf; | ||
| uint trailingZerosCount = bsf(_value); | ||
| // Because _value is non-zero, trailingZerosCount < sizeof(T)*8. |
There was a problem hiding this comment.
Now that you are using bsf, this comment seems rather out-of-place (bsf would have been undefined anyway). I'd say the above one is enough. And I liked the early-return version better.
|
@WalterBright @yebblies @MartinNowak: Any ideas about the 32 bit failures? Unless we are all missing something, this seems to be a misoptimization, ironically triggered by a fix to illegal code. |
JohanEngelen
force-pushed
the
outofboundshift
branch
from
d16cc66 to
d24f663
Compare
…aggressive optimization. The bug is triggered by the `assert(bitsSet(1).equal([0]));` unittest already present, with LDC+aggressive inlining and optimization.
JohanEngelen
force-pushed
the
outofboundshift
branch
from
d24f663 to
46f0d65
Compare
Current coverage is 88.67% (diff: 100%)@@ master #4509 diff @@
==========================================
Files 121 121
Lines 73827 73810 -17
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 65471 65454 -17
Misses 8356 8356
Partials 0 0
|
|
(rebased) |
Looks like a DMD code gen bug, I've reduced & filed it here: https://issues.dlang.org/show_bug.cgi?id=17034 |
|
The fix was merged and seems to have worked. Time to move this forward? |
There was a problem hiding this comment.
The fix was merged and seems to have worked.
Thanks a lot @WalterWaldron!
The changes look good to me.
Same here -> +1
As the fix was merged, it passes all CI checks, the change is fairly simply and nobody objected in the last seven days - I will go ahead and merge it, s.t. it will be part of the upcoming 2.073 release ;-) |
7 participants
A right-shift of a number of bits >= type's bits is invalid.
In this case, BitsSet was doing a
>>>=32on anint, leading to undefined behavior with aggressive optimizations in LDC. The solution in this PR is stopping further processing when the range has become empty by the popFront() call.The bug is triggered by the
assert(bitsSet(1).equal([0]));unittest already present, with LDC+aggressive inlining and optimization.